Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Checking ascribed expression types for existence #7796

Merged
merged 21 commits into from
Sep 14, 2023
Merged

Checking ascribed expression types for existence #7796

merged 21 commits into from
Sep 14, 2023

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Sep 12, 2023 •
edited
Loading

Pull Request Description

Fixes #6848 by:

  • making sure type in a:Xyz exists or yielding an error otherwise
  • making sure type in a:Integer is checked at runtime
  • simplifies conversions by using these inlined type check expressions

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • All code follows the
    Scala,
    Java,
  • All code has been tested:
    • Unit tests have been written where possible.

@JaroslavTulach JaroslavTulach requested a review from 4e6 as a code owner September 12, 2023 07:57
@JaroslavTulach JaroslavTulach self-assigned this Sep 12, 2023
@JaroslavTulach JaroslavTulach added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Sep 12, 2023
JaroslavTulach
JaroslavTulach commented Sep 12, 2023
View reviewed changes
engine/runtime/src/test/java/org/enso/interpreter/test/SignatureTest.java
m = a*b
add p m
p:Plus + m:Plus
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simplifies requesting conversions into desired types by using inline expressions p:Plus and m:Plus. Simpler than defining add (x:Plus) (y:Plus) method and requesting conversion of its arguments.

@enso-bot
Copy link

enso-bot bot commented Sep 13, 2023

Jaroslav Tulach reports a new STANDUP for yesterday (2023-09-12):

Progress: - x:Text ascription checks: #7796

Next Day: Finish x:Text ascription

@JaroslavTulach JaroslavTulach changed the title Checking ascribed expression types for existance Checking ascribed expression types for existence Sep 13, 2023
JaroslavTulach
JaroslavTulach commented Sep 13, 2023
View reviewed changes
test/Tests/src/Semantic/Conversion_Spec.enso Outdated
@@ -202,6 +212,19 @@ spec =
to_6 (v : Number & Decimal & Integer) = v
to_6 m . should_equal 1.5

Test.specify "Requesting Integer & Fool" <|
do_stuff (x : Integer & Fool) =
Copy link
Member Author

@JaroslavTulach JaroslavTulach Sep 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy of test provided by @radeusgd with slight modifications using x:Integer to hint the system.

Implementing == for multi value was a bit tricky - it is a method on Any and thus each value supports it. But at least the == and to_text work fine since 2bc7dd3 - the solution is to invoke all Any methods on the multi value as a whole.

@unfurl-links unfurl-links bot mentioned this pull request Sep 13, 2023
Merged
6 tasks
JaroslavTulach
JaroslavTulach commented Sep 13, 2023
View reviewed changes
...ntime/src/main/java/org/enso/interpreter/node/expression/builtin/number/integer/AddNode.java Outdated
TruffleObject that,
@CachedLibrary(limit = "3") InteropLibrary iop,
@Cached AddNode delegate) {
try {
Copy link
Member Author

@JaroslavTulach JaroslavTulach Sep 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will have to add this code into all arithmetic nodes, so @Akirathan, @hubertp & co. please review if dad0fb1 is OK.

hubertp
hubertp approved these changes Sep 13, 2023
View reviewed changes
Copy link
Collaborator

@hubertp hubertp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nits

engine/runtime/src/main/scala/org/enso/compiler/codegen/IrToTruffle.scala Outdated Show resolved Hide resolved
...ntime/src/main/java/org/enso/interpreter/node/expression/builtin/number/integer/AddNode.java Outdated Show resolved Hide resolved
engine/runtime/src/main/java/org/enso/interpreter/runtime/data/EnsoMultiValue.java Show resolved Hide resolved
JaroslavTulach
JaroslavTulach commented Sep 14, 2023
View reviewed changes
engine/runtime/src/test/java/org/enso/interpreter/test/NumbersTest.java Outdated
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;

@RunWith(Parameterized.class)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Akirathan, originally I was using @Theories, but after reading this stackoverflow post:

A Theory is wrong if a single test in it is wrong, according to the definition of a Theory. If your test cases don't follow this rule, it would be wrong to call them a "Theory".

and after realizing theories are in experimental package, I decided to switch to @Parametrized. The way to specify parameters() isn't as nice as in case of theories, but unlike the academical theory activists I believe running all the test is what real developers want. I seem to like proper error reports more then purity.

Don't you want to migrate to @Parametrized too?

JaroslavTulach
JaroslavTulach commented Sep 14, 2023
View reviewed changes
engine/runtime/src/test/java/org/enso/interpreter/test/NumbersTest.java Outdated
var r = new Random();
var ops = Arrays.asList(OPERATIONS).stream().map(
(op) -> new Object[] { op, r.nextLong(), switch (op) {
case " ^" -> r.nextLong(10);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a bit dangerous to let the right hand side argument of ^ to be Long.MAX_VALUE ;-)

@enso-bot enso-bot bot mentioned this pull request Sep 14, 2023
Closed
@JaroslavTulach JaroslavTulach force-pushed the wip/jtulach/AscribedExpressions_6848 branch from 597cb3f to 04a1d14 Compare September 14, 2023 07:48
@JaroslavTulach
Copy link
Member Author

I tried to update documentation, but this types document is just completely off:

Enso is a statically typed language

;-)

Atoms are the fundamental building blocks

atom is no longer a keyword. And so on, so on... This is my small documentation update.

@JaroslavTulach JaroslavTulach merged commit 30a62b9 into develop Sep 14, 2023
@JaroslavTulach JaroslavTulach deleted the wip/jtulach/AscribedExpressions_6848 branch September 14, 2023 12:09
@radeusgd radeusgd mentioned this pull request Sep 19, 2023
Closed
@JaroslavTulach JaroslavTulach mentioned this pull request Aug 24, 2024
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GregoryTravis GregoryTravis GregoryTravis approved these changes

@radeusgd radeusgd radeusgd approved these changes

@hubertp hubertp hubertp approved these changes

@4e6 4e6 Awaiting requested review from 4e6 4e6 is a code owner

@Akirathan Akirathan Awaiting requested review from Akirathan Akirathan is a code owner

@jdunkerley jdunkerley Awaiting requested review from jdunkerley jdunkerley is a code owner

Assignees

@JaroslavTulach JaroslavTulach

Labels
CI: Clean build required CI runners will be cleaned before and after this PR is built.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

x : Text is valid syntax, but doesn't check runtime type of the expression
4 participants
@JaroslavTulach @hubertp @radeusgd @GregoryTravis